fix(rate-limiter): populate tenant_id in GlobalContext for by_tenant rate limiting (#4343)#4380
Merged
brian-hussey merged 14 commits intomainfrom Apr 22, 2026
Merged
Conversation
gandhipratik203
requested review from
brian-hussey,
crivetimihai,
kevalmahajan and
madhav165
as code owners
jonpspri
force-pushed
the
feat/tenant-id-and-multi-tenant-tests
branch
from
29792be to
102e144
Compare
msureshkumar88
requested changes
Apr 22, 2026
Collaborator
There was a problem hiding this comment.
PR #4380 Code Review
Executive Summary
Recommendation: REQUEST CHANGES - Hardening opportunities found in tenant_id handling that could lead to Redis key injection attacks.
Critical Issues (Must Fix)
🔴 CRITICAL: Redis Key Injection Vulnerability
Location: mcpgateway/services/tool_service.py:151-157
Issue: The _extract_tenant_id_from_payload() function accepts any string value for team_id without validation or sanitization. This value is directly used in Redis key construction (rl:{tenant_id}:user:{email}:60), creating a Redis key injection vulnerability.
Attack Vector:
# Malicious team_id values:
team_id = "team:*:admin" # Could match/overwrite admin keys
team_id = "team\nDEL *" # Newline injection
team_id = "x" * 1000000 # Memory exhaustion
team_id = "../../../etc" # Path traversal attemptImpact:
- Severity: CRITICAL
- Tenant isolation bypass
- Potential data leakage across tenants
- Redis command injection
- Denial of service via memory exhaustion
Required Fix:
import re
def _extract_tenant_id_from_payload(team_id: Any) -> Optional[str]:
"""Extract and validate a tenant id from tool payload team_id value.
Validates format to prevent Redis key injection attacks.
Only allows alphanumeric, hyphens, and underscores.
"""
if team_id is None:
return None
if not isinstance(team_id, str):
logger.warning(
"Rejecting non-string team_id in tool payload: type=%s, value=%r",
type(team_id).__name__, team_id
)
return None
# Reject empty strings
if not team_id.strip():
logger.debug("Ignoring empty team_id in tool payload")
return None
# Enforce length limit (prevent memory exhaustion)
if len(team_id) > 128:
logger.warning(
"Rejecting oversized team_id (len=%d): %s...",
len(team_id), team_id[:50]
)
return None
# Validate format: only alphanumeric, hyphens, underscores
# This prevents Redis key injection via colons, newlines, wildcards
if not re.match(r'^[a-zA-Z0-9_-]+$', team_id):
logger.warning(
"Rejecting team_id with invalid characters: %r",
team_id
)
return None
return team_idTest Coverage Needed:
def test_extract_tenant_id_rejects_redis_injection_attempts():
"""Reject team_id values that could inject Redis commands."""
assert _extract_tenant_id_from_payload("team:*:admin") is None
assert _extract_tenant_id_from_payload("team\nDEL *") is None
assert _extract_tenant_id_from_payload("team;FLUSHALL") is None
assert _extract_tenant_id_from_payload("../../../etc") is None
assert _extract_tenant_id_from_payload("x" * 1000) is None
assert _extract_tenant_id_from_payload("") is None
assert _extract_tenant_id_from_payload(" ") is NoneHigh Priority Issues
🟠 HIGH: Missing Audit Logging for Tenant Context Changes
Location: tool_service.py:3904-3920, 4556-4578
Issue: When tenant_id is populated from the payload on fallback paths, there's no audit trail. This makes it impossible to track when/why tenant context was derived vs. propagated from middleware.
Impact:
- No forensic trail for tenant isolation failures
- Cannot detect if fallback path is being exploited
- Compliance issues (SOC2, GDPR require audit trails)
Required Fix:
# In _build_rust_tool_hook_global_context
if not hook_global_context.tenant_id and hook_tenant_id:
logger.info(
"Populating tenant_id from tool payload on fallback path",
extra={
"tenant_id": hook_tenant_id,
"request_id": hook_global_context.request_id,
"user": app_user_email,
"source": "tool_payload_fallback"
}
)
hook_global_context.tenant_id = hook_tenant_id🟠 HIGH: Race Condition in Integration Tests
Location: tests/integration/test_rate_limiter_multi_tenant.py:97-106
Issue: The _flush_rate_limiter_keys() function uses subprocess calls without proper error handling or synchronization. If Redis is under load, keys might not be fully deleted before the next test starts, causing test pollution.
Impact:
- Flaky tests
- False positives/negatives in CI
- Unreliable tenant isolation verification
Required Fix:
def _flush_rate_limiter_keys() -> None:
"""Delete any existing rl:* keys with verification."""
keys = _redis_keys("rl:*")
if not keys:
return
for key in keys:
result = subprocess.run(
["docker", "exec", "mcp-context-forge-redis-1", "redis-cli", "-n", "0", "DEL", key],
capture_output=True,
timeout=5,
check=False,
)
if result.returncode != 0:
pytest.fail(f"Failed to delete Redis key {key}: {result.stderr}")
# Verify deletion
remaining = _redis_keys("rl:*")
if remaining:
pytest.fail(f"Redis keys still present after flush: {remaining}")Medium Priority Issues
🟡 MEDIUM: Performance - Excessive Token Generation
Location: tests/integration/test_plugin_dynamic_behavior.py:68-74
Issue: _fresh_headers() is called in tight loops (e.g., _send_tool_burst calls it for every request). Each call performs a full login, creating unnecessary load on the auth system.
Impact:
- Slower test execution
- Unnecessary load on auth endpoints
- Could trigger rate limiting in production-like environments
Optimization:
@pytest.fixture(scope="module")
def auth_token():
"""Reuse auth token across tests in the module."""
return _get_session_token()
def _headers_with_token(token: str) -> dict:
"""Build headers with pre-fetched token."""
return {
"Authorization": f"Bearer {token}",
"Content-Type": "application/json",
"Accept": "application/json",
}
# Update _send_tool_burst to accept token parameter
def _send_tool_burst(server_id: str, tool_name: str, count: int, token: str) -> dict:
headers = _headers_with_token(token)
# ... rest of function🟡 MEDIUM: Missing Error Context in Integration Tests
Location: tests/integration/test_rate_limiter_dynamic_behavior.py:130-150
Issue: When tool calls fail, the error messages don't include enough context for debugging (server_id, tool_name, request payload).
Impact:
- Difficult to debug test failures in CI
- Wastes developer time
Improvement:
except requests.RequestException as e:
logger.error(
"Tool invocation failed",
extra={
"server_id": server_id,
"tool_name": tool_name,
"iteration": i,
"error": str(e)
}
)
errors += 1🟡 MEDIUM: Hardcoded Propagation Wait Time
Location: tests/helpers/integration_constants.py:4
Issue: PLUGIN_MODE_PROPAGATION_WAIT_SECONDS = 7 is arbitrary and could cause flaky tests if NGINX cache timing changes or under heavy load.
Impact:
- Flaky tests in CI
- Unnecessarily slow test execution
Improvement:
# Add exponential backoff with verification
def _wait_for_plugin_mode_propagation(expected_mode: str, max_wait: int = 10) -> None:
"""Wait for plugin mode to propagate across all replicas with verification."""
start = time.time()
while time.time() - start < max_wait:
state = _get_plugin_state()
plugins = {p["name"]: p for p in state.get("plugins", [])}
if plugins.get(PLUGIN_NAME, {}).get("mode") == expected_mode:
return
time.sleep(1)
pytest.fail(f"Plugin mode did not propagate to {expected_mode} within {max_wait}s")Low Priority Issues
🟢 LOW: Code Duplication Across Test Files
Location: Multiple test files have duplicate helper functions
Issue: _get_session_token(), _fresh_headers(), _is_gateway_running() are duplicated across:
test_plugin_dynamic_behavior.pytest_rate_limiter_dynamic_behavior.pytest_rate_limiter_multi_tenant.py
Refactoring Opportunity:
# tests/helpers/integration_helpers.py
def get_session_token(gateway_url: str, email: str, password: str) -> str:
"""Shared login helper for integration tests."""
# ... implementation
def fresh_headers(gateway_url: str, email: str, password: str) -> dict:
"""Shared auth headers helper."""
# ... implementation🟢 LOW: Missing Type Hints in Test Helpers
Location: Various test helper functions
Issue: Functions like _send_tool_burst, _redis_keys lack complete type hints.
Improvement:
from typing import Dict, List
def _send_tool_burst(
server_id: str,
tool_name: str,
count: int
) -> Dict[str, int]:
"""Send a burst of tool calls and return counts."""
# ... implementation
def _redis_keys(pattern: str) -> List[str]:
"""Query Redis for keys matching pattern."""
# ... implementationTesting Gaps
Missing Test Coverage
-
Tests:
- ❌ No tests for malicious
team_idvalues - ❌ No tests for Redis key injection attempts
- ❌ No tests for tenant isolation under concurrent load
- ❌ No tests for malicious
-
Error Path Tests:
- ❌ What happens if
team_idis present but Redis is down? - ❌ What happens if
team_idchanges mid-request? - ❌ What happens if plugin config is invalid?
- ❌ What happens if
-
Edge Cases:
- ❌ Unicode characters in
team_id - ❌ Very long
team_idvalues - ❌
team_idwith SQL injection patterns (defense in depth)
- ❌ Unicode characters in
-
Performance Tests:
- ❌ No load tests for tenant-scoped rate limiting
- ❌ No tests for Redis key space growth over time
Recommended Additional Tests
# tests/unit/mcpgateway/services/test_tool_service_tenant_id_security.py
class TestTenantIdSecurityValidation:
"""Security-focused tests for tenant_id extraction and validation."""
def test_rejects_redis_key_injection_colon(self):
"""Reject team_id containing colons (Redis key separator)."""
assert _extract_tenant_id_from_payload("team:admin:bypass") is None
def test_rejects_redis_wildcard_patterns(self):
"""Reject team_id containing Redis wildcard characters."""
assert _extract_tenant_id_from_payload("team*") is None
assert _extract_tenant_id_from_payload("team?") is None
assert _extract_tenant_id_from_payload("team[a-z]") is None
def test_rejects_newline_injection(self):
"""Reject team_id containing newlines (command injection)."""
assert _extract_tenant_id_from_payload("team\nDEL *") is None
assert _extract_tenant_id_from_payload("team\rFLUSHALL") is None
def test_rejects_oversized_team_id(self):
"""Reject team_id exceeding reasonable length limit."""
assert _extract_tenant_id_from_payload("x" * 1000) is None
def test_accepts_valid_alphanumeric_team_id(self):
"""Accept valid alphanumeric team_id with hyphens/underscores."""
assert _extract_tenant_id_from_payload("team-123") == "team-123"
assert _extract_tenant_id_from_payload("team_abc") == "team_abc"
assert _extract_tenant_id_from_payload("Team123") == "Team123"Implementation vs PR Description Match
✅ MATCHES: PR description accurately describes the changes
- Populates
tenant_idfromtool_payload["team_id"]✓ - Adds integration tests for multi-tenant rate limiting ✓
- Adds unit tests for tenant_id propagation ✓
- PR description doesn't mention validation of
team_id - PR description doesn't mention audit logging considerations
- PR description doesn't mention performance implications of test helpers
Dead Code Analysis
✅ NO DEAD CODE DETECTED - All added code is actively used in tests or production paths.
Logging & Exception Handling Gaps
Missing Logging
-
No logging when
team_idis ignored:# Current: Silent failure if team_id is not None and not isinstance(team_id, str): logger.debug(...) # Only debug level return None # Should be: Warning level for security events logger.warning( "Rejecting non-string team_id - potential security issue", extra={"type": type(team_id).__name__, "value": repr(team_id)} )
-
No logging when fallback path is taken:
- Should log when
plugin_global_contextis None (middleware didn't run) - Should log when
tenant_idis populated from payload
- Should log when
-
No structured logging in integration tests:
- Test failures don't include enough context for debugging
Missing Exception Handling
-
Subprocess calls in tests:
# Current: check=False but no error handling result = subprocess.run(..., check=False) # Should: Handle specific exceptions try: result = subprocess.run(..., check=True, timeout=5) except subprocess.TimeoutExpired: pytest.skip("Redis container not responding") except subprocess.CalledProcessError as e: pytest.fail(f"Redis command failed: {e.stderr}")
-
Network errors in integration tests:
- Should distinguish between connection errors, timeouts, and HTTP errors
- Should provide actionable error messages
Summary & Recommendations
Must Fix Before Merge (CRITICAL)
- ✅ Add input validation to
_extract_tenant_id_from_payload()to prevent Redis key injection - ✅ Add tests for malicious
team_idvalues - ✅ Add audit logging for tenant context population on fallback paths
Should Fix Before Merge (HIGH)
- ✅ Fix race condition in
_flush_rate_limiter_keys() - ✅ Add error handling to subprocess calls in tests
Consider for Follow-up (MEDIUM/LOW)
- Optimize token generation in integration tests
- Refactor duplicate test helpers into shared module
- Add exponential backoff for plugin mode propagation
- Add comprehensive error context to test failures
- Add type hints to test helper functions
Estimated Impact
- Risk: HIGH (Redis key injection vulnerability)
- Test Reliability: MEDIUM (race conditions, flaky waits)
- Performance: LOW (token generation overhead)
- Maintainability: LOW (code duplication)
Overall Recommendation: REQUEST CHANGES - Critical hardening issues must be addressed before merge.
Collaborator
Author
|
Thanks for the thorough review @msureshkumar88 — appreciate the depth here. Walking through the findings with my current understanding so we're aligned, and flagging what's landing in this PR vs. what we're proposing to defer as follow-ups (tracked in the PR body now). Redis key injection on Race condition in Audit logging on fallback path — this fallback fires on normal direct-invocation flows (not an exception path), so Test helper DRY extraction / token reuse / exponential backoff / type hints — agree these are reasonable improvements but scope-separate from the G1 tenant_id fix. Tracked in the "Review follow-ups" section of the PR body so they're not forgotten. Let me know if this framing works, or if there are specific items you'd like tightened further before this lands. |
msureshkumar88
previously approved these changes
Apr 22, 2026
Collaborator
msureshkumar88
left a comment
msureshkumar88
left a comment
There was a problem hiding this comment.
PR #4380 Follow-up Review - Response to Developer Comments
Re-evaluation Based on Developer Response
After reviewing the developer's response and threat model analysis, I'm updating my assessment.
Critical Issue Re-assessment: Redis Key Injection
Developer's Argument
"team_id flows from EmailTeam.id, which is a server-generated UUID hex (String(36) via uuid.uuid4().hex). The only way a malicious value could land in tool_payload['team_id'] would be via a platform-admin-created team record with a crafted id, which is already a trust-boundary violation upstream."
Updated Analysis
PARTIALLY ACCEPT with important caveats:
✅ Valid Points:
- Server-generated UUIDs are safe - If
team_idis always a UUID hex string fromuuid.uuid4().hex, it will only contain[0-9a-f-]characters, which are safe for Redis keys. - Trust boundary argument - If a platform admin can create arbitrary team IDs, the system is already compromised at a higher level.
- Redis binary-safe keys - Correct that Redis keys are binary-safe and don't interpret special characters as commands.
⚠️ Remaining Concerns:
1. Defense in Depth Principle
Even if the current code path is safe, defense in depth is a security best practice. The function name _extract_tenant_id_from_payload suggests it's extracting from potentially untrusted data. Future code changes could introduce new paths where team_id comes from different sources.
2. Code Clarity & Intent
The current implementation doesn't document the security assumption that team_id is always a UUID. This creates maintenance risk:
def _extract_tenant_id_from_payload(team_id: Any) -> Optional[str]:
"""Extract a valid tenant id from a raw tool payload team_id value."""
if team_id is not None and not isinstance(team_id, str):
logger.debug("Ignoring non-string team_id in tool payload: type=%s, value=%r", type(team_id).__name__, team_id)
return None
return team_id if team_id else NoneRecommendation: Add documentation and minimal validation:
def _extract_tenant_id_from_payload(team_id: Any) -> Optional[str]:
"""Extract and validate tenant id from tool payload team_id value.
Security Note: team_id is expected to be a server-generated UUID hex string
from EmailTeam.id. This function validates the format to ensure it's safe
for use in Redis key construction and to catch unexpected data shape changes.
Args:
team_id: Raw team_id value from tool payload (expected: UUID hex string)
Returns:
Validated tenant_id string, or None if invalid/missing
"""
if team_id is None:
return None
if not isinstance(team_id, str):
logger.warning(
"Unexpected non-string team_id in tool payload - possible data model change",
extra={"type": type(team_id).__name__, "value": repr(team_id)[:100]}
)
return None
# Defensive: Validate UUID hex format (36 chars, alphanumeric + hyphens)
# This catches unexpected data shape changes early rather than silently
# propagating potentially problematic values to Redis keys.
if not team_id or len(team_id) > 36 or not all(c in '0123456789abcdefABCDEF-' for c in team_id):
logger.warning(
"team_id does not match expected UUID hex format - rejecting",
extra={"team_id": team_id[:50]}
)
return None
return team_idSeverity Downgrade: CRITICAL → MEDIUM
Rationale:
- The threat model analysis is sound for the current implementation
- The risk is primarily about future code changes, not immediate exploitation
- Adding validation is still recommended but not blocking for this PR
- Can be addressed in a follow-up PR as the developer suggested
Race Condition Fix - VERIFIED ✅
Developer's Response
"Redis DEL is synchronous and atomic, so this isn't a race strictly speaking. That said, your point about making flush failures loud rather than silent is fair, so I've added a post-DEL verify assertion in f57f124"
Commit f57f124 Analysis
ACCEPTED - The fix adequately addresses the concern:
def _flush_rate_limiter_keys() -> None:
"""Delete any existing rl:* keys so we observe only keys from the current test."""
keys = _redis_keys("rl:*")
for key in keys:
subprocess.run(
["docker", "exec", "mcp-context-forge-redis-1", "redis-cli", "-n", "0", "DEL", key],
capture_output=True,
timeout=5,
check=False,
)
# Verify deletion completed
remaining = _redis_keys("rl:*")
if remaining:
pytest.fail(f"Redis keys still present after flush: {remaining}")Status: ✅ RESOLVED
Audit Logging - ACCEPT DEFERRAL
Developer's Response
"this fallback fires on normal direct-invocation flows (not an exception path), so info-level would produce a log entry per tool call. Would prefer to defer unless you feel a debug-level marker specifically would help on a future investigation."
Updated Assessment
ACCEPT with recommendation:
The developer's point about log volume is valid. However, a debug-level log entry would be valuable for troubleshooting without creating noise:
# Minimal addition - debug level only
if not hook_global_context.tenant_id and hook_tenant_id:
logger.debug(
"Populated tenant_id from tool payload on fallback path",
extra={"tenant_id": hook_tenant_id, "request_id": hook_global_context.request_id}
)
hook_global_context.tenant_id = hook_tenant_idSeverity: HIGH → LOW (optional enhancement)
Status: Can be deferred to follow-up PR
Other Issues - ACCEPT DEFERRAL
The following issues are reasonable to defer to follow-up PRs:
- Performance optimization (token reuse)
- Code duplication refactoring
- Type hints
- Exponential backoff for propagation waits
These are quality improvements but not blockers for the core functionality.
Updated Security Test Recommendations
Given the UUID hex assumption, the security tests should focus on data shape validation rather than injection attacks:
class TestTenantIdValidation:
"""Validate tenant_id extraction handles unexpected data shapes."""
def test_rejects_non_uuid_format(self):
"""Reject team_id that doesn't match UUID hex format."""
assert _extract_tenant_id_from_payload("not-a-uuid") is None
assert _extract_tenant_id_from_payload("team_123") is None
def test_rejects_oversized_string(self):
"""Reject team_id exceeding UUID length (36 chars)."""
assert _extract_tenant_id_from_payload("x" * 100) is None
def test_accepts_valid_uuid_hex(self):
"""Accept valid UUID hex format."""
valid_uuid = "550e8400-e29b-41d4-a716-446655440000"
assert _extract_tenant_id_from_payload(valid_uuid) == valid_uuid
def test_rejects_empty_string(self):
"""Reject empty string as tenant_id."""
assert _extract_tenant_id_from_payload("") is None
assert _extract_tenant_id_from_payload(" ") is NoneFinal Recommendation
CHANGE STATUS: REQUEST CHANGES → APPROVE WITH RECOMMENDATIONS
Rationale:
- ✅ Race condition fixed - Commit f57f124 adequately addresses the test reliability concern
- ✅ Threat model validated - Developer's analysis of the Redis injection risk is sound given the UUID constraint
- ✅ Core functionality correct - The tenant_id propagation logic is implemented correctly
- ✅ Test coverage adequate - Integration tests validate the end-to-end behavior
Remaining Recommendations (Non-blocking):
High Priority (Recommended for Follow-up PR)
- Add UUID format validation to
_extract_tenant_id_from_payload()with clear documentation of the security assumption - Add debug-level logging when tenant_id is populated on fallback paths
- Add data shape validation tests to catch unexpected team_id formats
Medium Priority (Can be deferred)
- Optimize token generation in integration tests
- Refactor duplicate test helpers
- Add comprehensive error context to test failures
Low Priority (Nice to have)
- Add type hints to test helper functions
- Implement exponential backoff for plugin mode propagation
Summary
The developer's response demonstrates a solid understanding of the threat model and security boundaries. The key insight is that team_id is server-generated (UUID hex), not user input, which significantly reduces the injection risk.
Key Changes:
- ✅ Race condition fixed in commit f57f124
⚠️ Redis injection downgraded from CRITICAL to MEDIUM (defense-in-depth recommendation)- ✅ Audit logging accepted as deferral (low priority)
- ✅ Other issues accepted as follow-up work
Approval Conditions Met:
- Core functionality is correct
- Critical security concerns are adequately addressed by the threat model
- Test coverage validates the behavior
- Remaining issues are quality improvements, not blockers
Recommendation: APPROVE with the understanding that follow-up PRs will address the defense-in-depth validation and logging enhancements.
Collaborator
Issue Coverage AnalysisDoes PR #4380 Solve Issue #4343?Answer: PARTIALLY ✅ What This PR Solves✅ G1 (HIGH) - Tenant ID Population - FULLY ADDRESSED
What This PR Does NOT Solve (By Design)❌ G2-G10 - Tracked in IBM/cpex-plugins#40 These gaps require changes in the cpex-plugins repository, not mcp-context-forge:
Dependency Chain
RecommendationIssue #4343 should remain open until IBM/cpex-plugins#40 is merged and G2-G10 are verified. This PR correctly implements its scope (G1) with excellent test coverage. Suggested issue update:
|
Collaborator
Author
|
Thanks for the re-review @msureshkumar88 — appreciate the quick turnaround. Agree with the scoping call: G1 lands here, G2-G10 live in IBM/cpex-plugins#40 (merged) and the follow-up #47 that bumps the wheel to 0.0.4. Once that releases and we bump I'll update #4343 with your suggested status structure so the progress is visible from the issue. Happy to cut a dedicated follow-up issue for the post-cpex-release verification if that's preferred over reopening/tracking via #4343. |
Collaborator
Author
Collaborator
Final Approval - Ready to Merge ✅Review CompleteAfter thorough review of the implementation, developer responses, and test results, this PR is approved and ready to merge. What Was Validated✅ Core Implementation (G1)
✅ Security Assessment
✅ Test Coverage (17 tests total)
✅ Loadtest Validation
✅ Bug Fixes
Scope ConfirmationThis PR correctly addresses:
Deferred to IBM/cpex-plugins#40:
Follow-up Recommendations (Non-blocking)Tracked for future PRs:
Merge ReadinessStatus: ✅ APPROVED - READY TO MERGE All blocking concerns addressed. The PR correctly implements its scope with comprehensive test coverage and proper security considerations. Issue #4343 should remain open until cpex-plugins#40 completes G2-G10. Great work @gandhipratik203 on the thorough implementation and thoughtful responses to review feedback! 🎉 |
Adds two integration test files that verify runtime plugin mode changes via the admin API affect actual plugin behavior on tool calls. test_plugin_dynamic_behavior.py (7 tests, all pass): Uses ReplaceBadWordsPlugin with fast-test-echo tool to verify text transformation starts/stops when the plugin is enabled/disabled at runtime. Covers enforce, disable, re-enable, cross-replica consistency, and full toggle cycle. test_rate_limiter_dynamic_behavior.py (6 tests, 5 pass, 1 known failure): Tests RateLimiterPlugin dynamic enable/disable with tool call bursts. Verifies rate limiting activates on first enable and Redis state propagation works. The disable→re-enable toggle cycle test fails — the rate limiter does not re-activate after being disabled and re-enabled within the same flow (G3 in #4343). Refs #4343 Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
…aths G1 from issue #4343. The happy path (HTTP request → HttpAuthMiddleware builds GlobalContext → get_current_user calls _propagate_tenant_id to fill tenant_id from request.state.team_id → tool_service reuses the context) already worked. The fallback branches in _build_rust_tool_hook_global_context and invoke_tool — which fire when middleware never ran — were still constructing GlobalContext with tenant_id hardcoded to None. Rate limiter's by_tenant dimension was therefore silently a no-op on those paths. Both fallbacks now derive tenant_id from tool_payload["team_id"] (already in scope — invoke_tool uses it at line 4463 for plugin context keying). Non-string values are ignored defensively. When plugin_global_context is supplied but carries tenant_id=None, the payload-derived value fills it in without overwriting an already-set value. Unit tests in tests/unit/mcpgateway/services/test_tool_service_tenant_id.py pin: happy propagation, absent team_id stays None, non-string team_id is ignored. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Pins the G1 / G2 gaps from issue #4343 end-to-end through the gateway HTTP flow, against a running docker-compose stack. Two tests under ``TestTenantIdFlowsToPlugin``: - ``test_tool_invocation_creates_rate_limit_keys_in_redis`` — sanity check. After enabling the rate limiter and making a real tool invocation, at least one ``rl:*`` key must exist in Redis. If this fails, the rate limiter isn't engaging on the tool path at all and everything else is meaningless. - ``test_rate_limit_keys_carry_tenant_prefix_when_tool_is_team_owned`` — G2 end-to-end. When the invoked tool belongs to a team, ``GlobalContext.tenant_id`` must flow through the tool service to the plugin and land as a prefix in the Redis key: ``rl:{team_id}:user:{email}:{window}``. Skips cleanly if the auto-detected server has no team owner. Current behaviour on a running stack built from the baseline image: the second test fails with keys in the unprefixed format (``rl:user:{email}:60``), which is the exact observable symptom of G1 — ``request.state.team_id`` isn't reaching the plugin's ``GlobalContext``. Will go green once the gateway redeploys with the tool-service fix from the previous commit in this PR, provided the deployment has admin team membership wired up via RBAC. Helpers: - Auth via the shared gateway session-token flow (same pattern as ``test_rate_limiter_dynamic_behavior.py``). - Redis inspection via ``docker exec`` against the ``mcp-context-forge-redis-1`` container so the test sidesteps any auth-config mismatch between the gateway's Redis client and a test-side client. - Autouse fixture disables the plugin and flushes ``rl:*`` keys between tests so each test starts from a clean slate. Integration-gated behind ``--with-integration`` and skipped when the gateway isn't reachable at ``http://localhost:8080``. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Diagnostic counterpart to ``test_rate_limiter_dynamic_behavior.py::test_disable_enable_disable_cycle``, which surfaces a 7/40 residual symptom at Step 1 (disabled after enforce): some requests still get HTTP 429 even after 7s of PROPAGATION_WAIT post mode=disabled. This new test mirrors the rate-limiter burst test exactly — same 3-step cycle, same BURST_SIZE=40, same PROPAGATION_WAIT via ``_set_plugin_mode`` — but against ``ReplaceBadWordsPlugin`` which has no Redis state, no Rust engine, no plugin-specific caching. If the rate-limiter symptom were framework-wide (mode invalidation not propagating fast enough across the 3 gateway replicas), bad-words would show the same partial-propagation pattern. If it's rate-limiter-specific (residual state in the plugin's Redis counters or Rust core), bad-words stays clean. Locally: bad-words sees 40/40 unchanged at Step 1, 40/40 transformed at Step 2, 40/40 unchanged at Step 3. All three steps clean. This isolates the rate-limiter's 7/40 residual as plugin-specific, not a framework mode-propagation issue. Also serves as a permanent regression pin on the framework's mode propagation — if the cross-replica invalidation ever breaks, this test is designed to surface it via the same 7/40 partial-convergence pattern. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
``TestRateLimiterToggleCycle::test_disable_enable_disable_cycle`` surfaced a 7/40 residual at Step 1 (disabled after enforce) that isn't reachable within this PR's scope. The parallel burst-cycle test we added for ReplaceBadWordsPlugin (TestBadWordsToggleBurst) hits the same gateway stack with the same shape and comes back 40/40 clean at every step, so framework mode-propagation is demonstrably fine. The residual is rate-limiter-specific and lives somewhere in the plugin shim or Rust core — worth its own investigation PR rather than keeping a failing test around as background noise. Removing entirely instead of marking xfail: the original plugin-manager work landed without this particular burst-cycle assertion, nobody relied on it as a contract, and keeping it xfailed adds a red line to every local integration run for no one's benefit. If someone revisits the rate-limiter lifecycle behaviour, they can recreate the test shape then — we already have a reusable counterpart pattern in TestBadWordsToggleBurst to copy from. The other five tests in this file (burst allowed when disabled, burst enforce, mode persisted in Redis, mode visible via admin API, mode reverts after disable) continue to pass and cover the ongoing behaviour that matters. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Jonathan Springer <jps@s390x.com>
Adds test case for line 4568 in tool_service.py where existing GlobalContext.tenant_id is preserved when already set by middleware, rather than being overwritten by tool_payload team_id. Achieves 100% coverage of tenant_id propagation logic in _build_rust_tool_hook_global_context method. Signed-off-by: Jonathan Springer <jps@s390x.com>
…nt-prefixed key layout
The G1/G2 fix in this branch moves rate-limiter keys for team-owned
tools from ``rl:{dim}:{id}:{win}`` to ``rl:{tenant_id}:{dim}:{id}:{win}``.
The scale test's Redis scans at ``_poll_redis_once`` and
``_detect_algorithm_from_redis`` used ``rl:user:*`` / ``rl:tenant:*``
globs that only match the pre-fix layout — post-fix they return zero
and the test reports silently-wrong metrics (Redis key delta of 0 even
though rate limiting is actively producing keys).
Added two helpers:
- ``_scan_rl_dimension(dim)`` — count keys across both layouts
- ``_scan_rl_sample_keys(dim)`` — sample keys for algorithm detection
Each unions ``rl:{dim}:*`` (unprefixed, single-tenant fallback) and
``rl:*:{dim}:*`` (tenant-prefixed, multi-tenant), so the test works
against pre-fix deployments, post-fix deployments, and mixed workloads
where the tool path produces prefixed keys and the prompt path
produces unprefixed keys.
Validated against the live 3-replica gateway:
- Before: old helpers return 0 keys despite 11 actual keys in Redis
- After: new helpers return 10 user keys + 1 tenant key = 11 ✓
- Algorithm detection: sample key found, Redis TYPE = string,
banner shows "fixed_window ✅ matches config"
- Other three rate-limiter locust files (locustfile_rate_limiter,
locustfile_rate_limiter_backend_correctness,
locustfile_rate_limiter_redis_capacity) don't scan Redis keys
and are unaffected — all three run cleanly end-to-end.
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
…/Pydantic
Additional hardening noticed while running the plugin-manager integration
suite against the rebuilt gateway image. Three validation-endpoint tests
were asserting ``status_code == 400`` but the server was returning 422 or
200 — the assertions were written against the RFC 7231 generic-400
convention, whereas FastAPI follows the RFC 4918 convention of returning
422 for request-body validation failures, and Pydantic's default ``bool``
type leniently coerces truthy strings like ``"yes"`` to ``True``.
Three fixes:
- ``test_missing_enabled_field`` — accept ``(400, 422)`` with a comment
explaining the FastAPI convention and why both are tolerated.
- ``test_non_boolean_enabled`` — renamed to
``test_truthy_string_enabled_coerced_to_bool`` and reworked to assert
the actual contract: ``{"enabled": "yes"}`` returns 200 and the
subsequent GET confirms the flag flipped to ``True``. Pinning the
coercion behaviour so it's not accidentally changed without a
deliberate ``StrictBool`` decision.
- ``test_invalid_mode_returns_400`` — renamed to
``test_invalid_mode_returns_4xx`` and widened to accept ``(400, 422)``
with the same FastAPI-convention comment.
No server-side changes. These tests were originally written in PR #4292;
this commit just brings their expectations in line with the framework's
actual documented behaviour.
Verified: 23/23 tests pass in ``test_plugin_runtime_management.py``
against the live 3-replica docker-compose stack.
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Addresses PR review feedback: add a post-DEL verify that confirms the rl:* keyspace is empty before a test proceeds, to make flush failures fail loudly rather than silently pollute the next test. Note: Redis DEL is synchronous and atomic, so this is a belt-and- suspenders assertion rather than a race-condition fix, but it gives debuggable failure if docker-exec or redis-cli ever returns a spurious success while the key survives. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
…mple scope The file holds one concrete case (ReplaceBadWordsPlugin + fast-test-echo) proving that mode changes via the admin API actually affect tool-call behaviour across gateway replicas. The previous filename implied broader coverage of 'dynamic plugin configuration', so rename to test_plugin_dynamic_behavior_bad_words.py and extend the docstring with a copy-and-adapt note for future per-plugin variants, plus an explicit note about the ReplaceBadWordsPlugin config dependency. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
…container name Addresses PR review follow-ups F1 and F3: F1: _extract_tenant_id_from_payload treated empty-string team_id as absent via the bare truthy check, with no signal the contract was intentional. Extend the docstring to pin the rule so a future reader doesn't decide the falsy-string branch is an oversight and let empty values through — a zero-length tenant prefix would collapse tenant-scoped Redis keys onto the unscoped layout and silently break isolation. F3: the integration test docker-exec calls hardcoded the Redis container name (mcp-context-forge-redis-1), which is the compose default but derives from the project-name prefix. A checkout under a different directory name or a custom COMPOSE_PROJECT_NAME silently skips every test here instead of failing loudly. Read the container name from a REDIS_CONTAINER_NAME env var with the current value as the default, so the common case stays zero-config while non-default deployments have a documented escape hatch. Matches the DOCKER_REDIS_CONTAINER pattern already used in the locustfiles. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
…nto shared helper Both _build_rust_tool_hook_global_context and invoke_tool have the same fill-missing block for server_id, user, and tenant_id on an already- existing GlobalContext. The blocks drifted only in variable names, not semantics. Extract the shared logic into _apply_tool_payload_to_global_context so the two call sites stay in lockstep, and the helper is covered by a single unit test rather than needing per-site exercise of identical logic (which was the root of the diff-coverage gap at tool_service.py:4572). Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
gandhipratik203
force-pushed
the
feat/tenant-id-and-multi-tenant-tests
branch
from
b70a6af to
5be6186
Compare
Picks up the production hardening released in cpex-plugins 0.0.4 (PR IBM/cpex-plugins#40, released as rate-limiter-v0.0.4): tenant- scoped Redis keys, strict fail_mode validation, initialize/shutdown lifecycle hooks, and parse_rate bounds. Paired with the G1 tenant_id propagation fix already on this branch, this unblocks end-to-end by_user and by_tenant enforcement across multi-tenant deployments. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
brian-hussey
approved these changes
Apr 22, 2026
Member
brian-hussey
left a comment
brian-hussey
left a comment
There was a problem hiding this comment.
Based on others approval merging this now.
gcgoncalves
pushed a commit
that referenced
this pull request
Apr 23, 2026
…rate limiting (#4343) (#4380) * test(plugins): integration tests for dynamic plugin behavior Adds two integration test files that verify runtime plugin mode changes via the admin API affect actual plugin behavior on tool calls. test_plugin_dynamic_behavior.py (7 tests, all pass): Uses ReplaceBadWordsPlugin with fast-test-echo tool to verify text transformation starts/stops when the plugin is enabled/disabled at runtime. Covers enforce, disable, re-enable, cross-replica consistency, and full toggle cycle. test_rate_limiter_dynamic_behavior.py (6 tests, 5 pass, 1 known failure): Tests RateLimiterPlugin dynamic enable/disable with tool call bursts. Verifies rate limiting activates on first enable and Redis state propagation works. The disable→re-enable toggle cycle test fails — the rate limiter does not re-activate after being disabled and re-enabled within the same flow (G3 in #4343). Refs #4343 Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * fix(tool-service): populate tenant_id from tool_payload on fallback paths G1 from issue #4343. The happy path (HTTP request → HttpAuthMiddleware builds GlobalContext → get_current_user calls _propagate_tenant_id to fill tenant_id from request.state.team_id → tool_service reuses the context) already worked. The fallback branches in _build_rust_tool_hook_global_context and invoke_tool — which fire when middleware never ran — were still constructing GlobalContext with tenant_id hardcoded to None. Rate limiter's by_tenant dimension was therefore silently a no-op on those paths. Both fallbacks now derive tenant_id from tool_payload["team_id"] (already in scope — invoke_tool uses it at line 4463 for plugin context keying). Non-string values are ignored defensively. When plugin_global_context is supplied but carries tenant_id=None, the payload-derived value fills it in without overwriting an already-set value. Unit tests in tests/unit/mcpgateway/services/test_tool_service_tenant_id.py pin: happy propagation, absent team_id stays None, non-string team_id is ignored. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * test(plugins): multi-tenant integration tests for rate limiter (G1 + G2) Pins the G1 / G2 gaps from issue #4343 end-to-end through the gateway HTTP flow, against a running docker-compose stack. Two tests under ``TestTenantIdFlowsToPlugin``: - ``test_tool_invocation_creates_rate_limit_keys_in_redis`` — sanity check. After enabling the rate limiter and making a real tool invocation, at least one ``rl:*`` key must exist in Redis. If this fails, the rate limiter isn't engaging on the tool path at all and everything else is meaningless. - ``test_rate_limit_keys_carry_tenant_prefix_when_tool_is_team_owned`` — G2 end-to-end. When the invoked tool belongs to a team, ``GlobalContext.tenant_id`` must flow through the tool service to the plugin and land as a prefix in the Redis key: ``rl:{team_id}:user:{email}:{window}``. Skips cleanly if the auto-detected server has no team owner. Current behaviour on a running stack built from the baseline image: the second test fails with keys in the unprefixed format (``rl:user:{email}:60``), which is the exact observable symptom of G1 — ``request.state.team_id`` isn't reaching the plugin's ``GlobalContext``. Will go green once the gateway redeploys with the tool-service fix from the previous commit in this PR, provided the deployment has admin team membership wired up via RBAC. Helpers: - Auth via the shared gateway session-token flow (same pattern as ``test_rate_limiter_dynamic_behavior.py``). - Redis inspection via ``docker exec`` against the ``mcp-context-forge-redis-1`` container so the test sidesteps any auth-config mismatch between the gateway's Redis client and a test-side client. - Autouse fixture disables the plugin and flushes ``rl:*`` keys between tests so each test starts from a clean slate. Integration-gated behind ``--with-integration`` and skipped when the gateway isn't reachable at ``http://localhost:8080``. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * test(plugins): burst-mode toggle cycle against ReplaceBadWordsPlugin Diagnostic counterpart to ``test_rate_limiter_dynamic_behavior.py::test_disable_enable_disable_cycle``, which surfaces a 7/40 residual symptom at Step 1 (disabled after enforce): some requests still get HTTP 429 even after 7s of PROPAGATION_WAIT post mode=disabled. This new test mirrors the rate-limiter burst test exactly — same 3-step cycle, same BURST_SIZE=40, same PROPAGATION_WAIT via ``_set_plugin_mode`` — but against ``ReplaceBadWordsPlugin`` which has no Redis state, no Rust engine, no plugin-specific caching. If the rate-limiter symptom were framework-wide (mode invalidation not propagating fast enough across the 3 gateway replicas), bad-words would show the same partial-propagation pattern. If it's rate-limiter-specific (residual state in the plugin's Redis counters or Rust core), bad-words stays clean. Locally: bad-words sees 40/40 unchanged at Step 1, 40/40 transformed at Step 2, 40/40 unchanged at Step 3. All three steps clean. This isolates the rate-limiter's 7/40 residual as plugin-specific, not a framework mode-propagation issue. Also serves as a permanent regression pin on the framework's mode propagation — if the cross-replica invalidation ever breaks, this test is designed to surface it via the same 7/40 partial-convergence pattern. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * test(rate-limiter): drop the enforce→disabled toggle burst test ``TestRateLimiterToggleCycle::test_disable_enable_disable_cycle`` surfaced a 7/40 residual at Step 1 (disabled after enforce) that isn't reachable within this PR's scope. The parallel burst-cycle test we added for ReplaceBadWordsPlugin (TestBadWordsToggleBurst) hits the same gateway stack with the same shape and comes back 40/40 clean at every step, so framework mode-propagation is demonstrably fine. The residual is rate-limiter-specific and lives somewhere in the plugin shim or Rust core — worth its own investigation PR rather than keeping a failing test around as background noise. Removing entirely instead of marking xfail: the original plugin-manager work landed without this particular burst-cycle assertion, nobody relied on it as a contract, and keeping it xfailed adds a red line to every local integration run for no one's benefit. If someone revisits the rate-limiter lifecycle behaviour, they can recreate the test shape then — we already have a reusable counterpart pattern in TestBadWordsToggleBurst to copy from. The other five tests in this file (burst allowed when disabled, burst enforce, mode persisted in Redis, mode visible via admin API, mode reverts after disable) continue to pass and cover the ongoing behaviour that matters. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * fix: address rate limiter review findings Signed-off-by: Jonathan Springer <jps@s390x.com> * test(tool-service): add coverage for tenant_id preservation branch Adds test case for line 4568 in tool_service.py where existing GlobalContext.tenant_id is preserved when already set by middleware, rather than being overwritten by tool_payload team_id. Achieves 100% coverage of tenant_id propagation logic in _build_rust_tool_hook_global_context method. Signed-off-by: Jonathan Springer <jps@s390x.com> * test(loadtest): adapt rate-limiter scale-test Redis scans to the tenant-prefixed key layout The G1/G2 fix in this branch moves rate-limiter keys for team-owned tools from ``rl:{dim}:{id}:{win}`` to ``rl:{tenant_id}:{dim}:{id}:{win}``. The scale test's Redis scans at ``_poll_redis_once`` and ``_detect_algorithm_from_redis`` used ``rl:user:*`` / ``rl:tenant:*`` globs that only match the pre-fix layout — post-fix they return zero and the test reports silently-wrong metrics (Redis key delta of 0 even though rate limiting is actively producing keys). Added two helpers: - ``_scan_rl_dimension(dim)`` — count keys across both layouts - ``_scan_rl_sample_keys(dim)`` — sample keys for algorithm detection Each unions ``rl:{dim}:*`` (unprefixed, single-tenant fallback) and ``rl:*:{dim}:*`` (tenant-prefixed, multi-tenant), so the test works against pre-fix deployments, post-fix deployments, and mixed workloads where the tool path produces prefixed keys and the prompt path produces unprefixed keys. Validated against the live 3-replica gateway: - Before: old helpers return 0 keys despite 11 actual keys in Redis - After: new helpers return 10 user keys + 1 tenant key = 11 ✓ - Algorithm detection: sample key found, Redis TYPE = string, banner shows "fixed_window ✅ matches config" - Other three rate-limiter locust files (locustfile_rate_limiter, locustfile_rate_limiter_backend_correctness, locustfile_rate_limiter_redis_capacity) don't scan Redis keys and are unaffected — all three run cleanly end-to-end. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * test(plugins): align runtime-management validation tests with FastAPI/Pydantic Additional hardening noticed while running the plugin-manager integration suite against the rebuilt gateway image. Three validation-endpoint tests were asserting ``status_code == 400`` but the server was returning 422 or 200 — the assertions were written against the RFC 7231 generic-400 convention, whereas FastAPI follows the RFC 4918 convention of returning 422 for request-body validation failures, and Pydantic's default ``bool`` type leniently coerces truthy strings like ``"yes"`` to ``True``. Three fixes: - ``test_missing_enabled_field`` — accept ``(400, 422)`` with a comment explaining the FastAPI convention and why both are tolerated. - ``test_non_boolean_enabled`` — renamed to ``test_truthy_string_enabled_coerced_to_bool`` and reworked to assert the actual contract: ``{"enabled": "yes"}`` returns 200 and the subsequent GET confirms the flag flipped to ``True``. Pinning the coercion behaviour so it's not accidentally changed without a deliberate ``StrictBool`` decision. - ``test_invalid_mode_returns_400`` — renamed to ``test_invalid_mode_returns_4xx`` and widened to accept ``(400, 422)`` with the same FastAPI-convention comment. No server-side changes. These tests were originally written in PR #4292; this commit just brings their expectations in line with the framework's actual documented behaviour. Verified: 23/23 tests pass in ``test_plugin_runtime_management.py`` against the live 3-replica docker-compose stack. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * test(rate-limiter): verify rl:* flush completes before proceeding Addresses PR review feedback: add a post-DEL verify that confirms the rl:* keyspace is empty before a test proceeds, to make flush failures fail loudly rather than silently pollute the next test. Note: Redis DEL is synchronous and atomic, so this is a belt-and- suspenders assertion rather than a race-condition fix, but it gives debuggable failure if docker-exec or redis-cli ever returns a spurious success while the key survives. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * test(plugins): rename dynamic-behavior test to reflect its worked-example scope The file holds one concrete case (ReplaceBadWordsPlugin + fast-test-echo) proving that mode changes via the admin API actually affect tool-call behaviour across gateway replicas. The previous filename implied broader coverage of 'dynamic plugin configuration', so rename to test_plugin_dynamic_behavior_bad_words.py and extend the docstring with a copy-and-adapt note for future per-plugin variants, plus an explicit note about the ReplaceBadWordsPlugin config dependency. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * test(rate-limiter): pin empty-string contract and parameterize Redis container name Addresses PR review follow-ups F1 and F3: F1: _extract_tenant_id_from_payload treated empty-string team_id as absent via the bare truthy check, with no signal the contract was intentional. Extend the docstring to pin the rule so a future reader doesn't decide the falsy-string branch is an oversight and let empty values through — a zero-length tenant prefix would collapse tenant-scoped Redis keys onto the unscoped layout and silently break isolation. F3: the integration test docker-exec calls hardcoded the Redis container name (mcp-context-forge-redis-1), which is the compose default but derives from the project-name prefix. A checkout under a different directory name or a custom COMPOSE_PROJECT_NAME silently skips every test here instead of failing loudly. Read the container name from a REDIS_CONTAINER_NAME env var with the current value as the default, so the common case stays zero-config while non-default deployments have a documented escape hatch. Matches the DOCKER_REDIS_CONTAINER pattern already used in the locustfiles. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * refactor(tool-service): hoist tool-payload GlobalContext enrichment into shared helper Both _build_rust_tool_hook_global_context and invoke_tool have the same fill-missing block for server_id, user, and tenant_id on an already- existing GlobalContext. The blocks drifted only in variable names, not semantics. Extract the shared logic into _apply_tool_payload_to_global_context so the two call sites stay in lockstep, and the helper is covered by a single unit test rather than needing per-site exercise of identical logic (which was the root of the diff-coverage gap at tool_service.py:4572). Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * chore(deps): bump cpex-rate-limiter to 0.0.4 Picks up the production hardening released in cpex-plugins 0.0.4 (PR IBM/cpex-plugins#40, released as rate-limiter-v0.0.4): tenant- scoped Redis keys, strict fail_mode validation, initialize/shutdown lifecycle hooks, and parse_rate bounds. Paired with the G1 tenant_id propagation fix already on this branch, this unblocks end-to-end by_user and by_tenant enforcement across multi-tenant deployments. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> --------- Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> Signed-off-by: Jonathan Springer <jps@s390x.com> Co-authored-by: Jonathan Springer <jps@s390x.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Populates
GlobalContext.tenant_idon the tool-service fallback paths so the rate limiter'sby_tenantdimension stops being a silent no-op, and adds integration test coverage that validates the tenant dimension end-to-end through the full gateway HTTP flow.Closes the main-repo half of issue #4343 (G1). The cpex plugin half is in IBM/cpex-plugins#40; this PR is kept as draft until that one merges and a cpex release is published.
Gap closed
Gap 1 (HIGH) —
GlobalContext.tenant_idwas hardcoded toNoneon the two fallback branches intool_service.py(_build_rust_tool_hook_global_contextandinvoke_tool) that fire when middleware didn't run. The happy path (HTTP →HttpAuthMiddleware→auth.get_current_user→_propagate_tenant_id) already handled this, but the direct-invocation and fresh-context paths lefttenant_idatNone, silently skippingby_tenantrate limiting and producing unprefixed Redis keys that collide across tenants.Both fallback branches now derive
tenant_idfromtool_payload["team_id"]— which is already in scope at both call sites (see_tool_team_id = tool_payload.get("team_id")attool_service.py:4463). Non-stringteam_idvalues are ignored defensively. Whenplugin_global_contextis supplied but carriestenant_id=None, the payload-derived value fills it in without overwriting an already-set value.Architecture
tenant_idflow — beforetenant_idflow — afterTest results
Unit tests (
test_tool_service_tenant_id.py)Three pins on the fallback-path behaviour:
test_build_rust_tool_hook_global_context_propagates_team_id_as_tenant_idtest_build_rust_tool_hook_global_context_tenant_id_none_when_team_id_absenttest_build_rust_tool_hook_global_context_non_string_team_id_is_ignoredRun:
uv run pytest tests/unit/mcpgateway/services/test_tool_service_tenant_id.py -v— 3 passed.Each test was verified RED before the fix and GREEN after — the propagation test was the central red→green cycle.
Integration tests (
test_rate_limiter_multi_tenant.py)Two end-to-end tests via the gateway HTTP flow, gated behind
--with-integration:test_tool_invocation_creates_rate_limit_keys_in_redistest_rate_limit_keys_carry_tenant_prefix_when_tool_is_team_ownedRun:
uv run pytest tests/integration/test_rate_limiter_multi_tenant.py --with-integration -v— 2 passed.End-to-end validation evidence: after an admin-scoped tool invocation against a freshly-rebuilt gateway (with this PR's changes + cpex #40 pinned via git ref), the Redis backend showed:
Both keys carry the tenant prefix. The
tenant:dimension key also exists, confirmingby_tenantrate limiting is fully functional on the happy path — not just theby_userdimension under a tenant scope.Additional hardening —
test_plugin_runtime_management.pyvalidation assertions aligned with FastAPI/PydanticThree validation-endpoint tests originally from #4292 were asserting
status_code == 400but the server was returning 422 or 200 — the assertions were written against the RFC 7231 generic-400 convention, whereas FastAPI follows the RFC 4918 convention of 422 for body-validation failures, and Pydantic's defaultboolcoerces truthy strings.test_missing_enabled_field== 400(failed — actual 422)in (400, 422)with convention commenttest_non_boolean_enabled(renamedtest_truthy_string_enabled_coerced_to_bool)== 400(failed — actual 200, Pydantic coerced"yes"→True)plugins_globally_enabled == True; pins the coercion contracttest_invalid_mode_returns_400(renamedtest_invalid_mode_returns_4xx)== 400(failed — actual 422)in (400, 422)with convention commentNo server-side changes. Full file runs 23/23 green against the live gateway after the fix (was 20/23).
Dynamic behaviour tests (
test_rate_limiter_dynamic_behavior.py)Five tests exercising runtime mode toggles against the rate limiter end-to-end (docker-compose stack).
test_burst_all_allowed_when_disabledtest_burst_some_blocked_when_enforcingtest_mode_stored_in_redistest_mode_visible_in_admin_api_after_changetest_mode_reverts_in_admin_api_after_disableA sixth burst-cycle test (
test_disable_enable_disable_cycle) was present on the parent branch and showed a 7/40 residual at Step 1 (disabled after enforce). The parallel bad-words burst test added in this PR (see below) hits the same gateway stack with the same shape and comes back 40/40 clean, so framework mode-propagation is fine — the residual is rate-limiter-specific and worth its own investigation rather than keeping a failing test around. Dropped from this file; can be recreated as part of a dedicated follow-up.Load tests — rate-limiter locust files against live 3-replica gateway
All four rate-limiter locust files validated against the rebuilt gateway image (with G1/G2 fix + cpex PR #40 tenant-scoping). Short runs (30s each, small user counts) were sufficient to exercise the Redis + tool-call paths.
locustfile_rate_limiter.pylocustfile_rate_limiter_backend_correctness.pylocustfile_rate_limiter_redis_capacity.pylocustfile_rate_limiter_scale.py(modified here)fixed_window [string]Redis key layouts observed — both coexist cleanly in the same Redis instance:
rl:{tenant_id}:user:{email}:60(tenant-prefixed, post-G1)rl:{tenant_id}:tenant:{tenant_id}:60(by_tenant dimension, now functional)rl:user:anonymous:60(unprefixed —prompt_service.pyalready handledtenant_idcorrectly; the capacity test doesn't supply one)The scale-test helper update (
_scan_rl_dimension/_scan_rl_sample_keys) unionsrl:{dim}:*andrl:*:{dim}:*, so all three layouts are counted regardless of which path produced them.Before the helper fix:
_scan_redis_pattern("rl:user:*")returned 0 keys against the post-fix deployment even with 11 actual keys in Redis — silent metric corruption.After the helper fix: returns 11, matching ground truth.
Framework mode-propagation regression pin (
test_plugin_dynamic_behavior.py::TestBadWordsToggleBurst)One burst-cycle test against
ReplaceBadWordsPluginmirroring the rate-limiter burst shape (3-step disabled → enforce → disabled cycle, BURST_SIZE=40, same PROPAGATION_WAIT):This test proves the plugin-manager's mode-invalidation path works cleanly across three gateway replicas within PROPAGATION_WAIT. Also serves as a permanent regression pin — if cross-replica invalidation ever breaks, this test is designed to surface it before silent failures in production.
Dependencies
context_prefixfix won't prefix keys.Follow-ups
cpex-rate-limiterpin inpyproject.tomlonce cpex 0.0.4 is released (separate PR, trivial chore).Review follow-ups (deferred)
From PR review feedback that is either out-of-scope for this PR's narrow goal (G1 tenant_id population) or better addressed at a different layer. Tracking here so they aren't lost:
team_idformat validation.team_idis server-derived fromEmailTeam.id(UUID hexString(36)), not request body, so there is no user-controlled-input path into the Redis key. If additional format validation is desired, it belongs at the team-creation schema boundary rather than the rate-limiter fallback — broader scope that merits its own PR.infolevel on every fallback tool invocation would be log spam (fallback is the normal direct-invocation path, not an exception path). Adebug-level hook is reasonable if/when needed.test_plugin_dynamic_behavior.py/test_rate_limiter_dynamic_behavior.py/test_rate_limiter_multi_tenant.py(local copies are intentional for file readability for now)._send_tool_burst,_redis_keys, etc.). Cosmetic; defer.cpex-rate-limiterpin bump inpyproject.tomlonce cpex-plugins 0.0.4 is released (tracked separately; see Dependencies section).